Do you want to support disabling this setting? Per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age, the value -1 disables the timeout.

If so, the standard for other Contour timeouts is that the input string infinity disables the timeout. The timeout.Parse function already handles parsing this.

In most other contexts "disables the timeout" means that the operation can (in principle) take forever, but -1 here means that the max age never applies, so max-age of "infinity" seems misleading. For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

Both 0 and -1 mean disable caching for the preflight request but I decided to support just the 0 value. I found tricky to handle the -1 taking into account that the durations are expressed in the Go duration format. If we wanted to support -1 how would we do that? -1s disables caching but -1h is an invalid value? I'd rather allow negative values and assume that all of them disable the cache.

If you want to support disabling MaxAge (see other comment), then you'd also want to check for cp.MaxAge.IsDisabled() here, and if true, pass a value of -1 to Envoy.

yeh, -1 here means an immediate timeout, not no timeout.

Now I'm doing that but for the 0 value.

Since MaxAge is parsed by timeout.Setting, please include the boilerplate format description (this will be consolidated into validation regex in the future):

// MaAge durations are expressed in the Go [Duration format](https://godoc.org/time#ParseDuration).
// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
// The string "infinity" is also a valid input and specifies no timeout.
// A value of "0s" will be treated as if the field were not set, i.e. by using Envoy's default behavior.

done!

These strings end up being joined with , and passed directly to one envoy filter, so we should add validation. Here's what I suggest (the pattern approximates the tchar spec from RFC 7230 sec 3.2.6):

// +kubebuilder:validation:Pattern=[a-zA-Z0-9!#$%&'*+.^_`|~-]+
type CorsHeaderValue string

type CorsPolicy struct {
    AllowOrigin []CORSHeaderValue
    AllowMethods []CORSHeaderValue
    AllowHeaders []CORSHeaderValue
    ExposeHeaders []CORSHeaderValue
}

```

We can't apply this validation pattern for the AllowOrigin header as URLs like http://example.com are valid values. @skriss should I apply it for the rest of the headers and leave AllowOrigin as a []string?

In most other contexts "disables the timeout" means that the operation can (in principle) take forever, but -1 here means that the max age never applies, so max-age of "infinity" seems misleading. For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

Reading the envoy docs, it's not clear to me whether simply adding the filter activates CORS. The filter_enabled field says:

If neither enabled, filter_enabled, nor shadow_enabled are specified, the CORS filter will be enabled for 100% of the requests.

So does that mean that when CORS policy is omitted, we need to generate a CorsPolicy_Enabled message with Enabled=false?

After testing it, I think that if we don't set the EnabledSpecifier property of &envoy_api_v2_route.CorsPolicy, the CORS filter will be enabled by default. When CORS configuration is omitted, we are not setting any CORS policy to the VirtualHost therefore, the filter is disabled.

Rather than enable this a fractional 100%, would it be simpler to do:

EnabledSpecified: &CorsPolicy_Enabled {
    Enabled: protobuf.Bool(true),
}

I think that we should keep using filter_enabled as enabled only applies to routes and according to the docs, it is deprecated:

enabled
(BoolValue) Specifies if the CORS filter is enabled. Defaults to true. Only effective on route.
Attention
This field is deprecated. Set the filter_enabled field instead.

I've ended up leaving the EnabledSpecified property unset as the CORS filter is enabled by default (see my comment above).

yeh, -1 here means an immediate timeout, not no timeout.

It's worth a comment here about that since it's non-obvious.

nit: /possitive/positive/

done!